-
Notifications
You must be signed in to change notification settings - Fork 502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
4837 single user email import #5582
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! It's a complicated machine with a lot of parts and I think we need to really make it as understandable as possible.
I've left some comments in places but it would be too much to repeat it every time.
Here are some general code style comments:
- use for-of loop instead of foreach whenever possible
- use const instead of let whenever possible
- collapse fields and constructors whenever possible
- prefer plain objects/interfaces to classes, especially in remote invocation context
- do not use streams unless you really need them, prefer plain fields
- do not prepare view attributes before rendering, just use them whenever you render a component
Besides that we need a lot of JSDOC comments for different methods/classes. I know there's another doc being prepared and it's good to have high-level overview there but it should still be possible to grasp what the class does without referring to it.
@@ -23,7 +21,7 @@ export async function runDevBuild({ stage, host, desktop, clean, ignoreMigration | |||
if (ignoreMigrations) { | |||
console.warn("CAUTION: Offline migrations are not being validated.") | |||
} else { | |||
checkOfflineDatabaseMigrations() | |||
//checkOfflineDatabaseMigrations() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to just add a migration that drops the db if that's what you need?
) {} | ||
|
||
async initializeImapImport(initializeParams: InitializeImapImportParams): Promise<ImportImapAccountSyncState> { | ||
const mailGroupId = this.userFacade.getGroupId(GroupType.Mail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we passed the mail group in, shared mailboxes are a thing after all
importImapAccountSyncStateId: Id, | ||
parentFolderId: IdTuple | null, | ||
): Promise<ImportImapFolderSyncState | undefined> { | ||
if (imapMailbox.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens when there's no name?
const mailGroupId = this.userFacade.getGroupId(GroupType.Mail) | ||
const mailGroupKey = this.userFacade.getGroupKey(mailGroupId) | ||
const sk = aes128RandomKey() | ||
const service = createImportMailPostIn() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please pass things directly into create
call rather than assigning them (also generally)
/** | ||
* Uploads the given data files or sets the file if it is already existing and returns all ImportAttachments | ||
*/ | ||
async _createAddedImportAttachments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it underscored and not private?
value: vnode.attrs.data.model.imapAccountHost(), | ||
oninput: vnode.attrs.data.model.imapAccountHost, | ||
helpLabel: () => { | ||
const host = vnode.attrs.data.model.imapAccountHost() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is host for here?
} | ||
}, | ||
} | ||
const imapAccountPortAttrs: TextFieldAttrs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a number field?
|
||
private async requestImapImportAccountSyncState() { | ||
const importImapAccountSyncState = await locator.imapImporterFacade.loadImportImapAccountSyncState() | ||
const rootImportMailFolder = await locator.imapImporterFacade.loadRootImportFolder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can it fail? what happens?
private imapImportState: stream<ImapImportState> | ||
|
||
constructor() { | ||
this.imapAccountHost = stream<string>("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
steams are also unnecessary here
@@ -7,7 +7,7 @@ | |||
"outDir": "build/dist", | |||
"incremental": true | |||
}, | |||
"include": ["src/", "libs/*.ts", "types/*.d.ts"], | |||
"include": ["src/", "libs/*.ts", "types/*.d.ts", "node_modules/imapflow/lib/types.d.ts"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other types are picked up automatically, why is this necessary?;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! It's a complicated machine with a lot of parts and I think we need to really make it as understandable as possible.
I've left some comments in places but it would be too much to repeat it every time.
Here are some general code style comments:
- use for-of loop instead of foreach whenever possible
- use const instead of let whenever possible
- collapse fields and constructors whenever possible
- prefer plain objects/interfaces to classes, especially in remote invocation context
- do not use streams unless you really need them, prefer plain fields
- do not prepare view attributes before rendering, just use them whenever you render a component
Besides that we need a lot of JSDOC comments for different methods/classes. I know there's another doc being prepared and it's good to have high-level overview there but it should still be possible to grasp what the class does without referring to it.
import { ApplicationWindow } from "../ApplicationWindow.js" | ||
|
||
export class DesktopImapImportSystemFacade implements ImapImportSystemFacade, AdSyncEventListener { | ||
constructor(private readonly win: ApplicationWindow) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't take ApplicationWindow, rather keep a list/record of active ImapImportFacades (maybe per userId or mailbox id)
remove imapImportFacade from ApplicationWindow
host: string | ||
port: string | ||
username: string | ||
password: string | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we type this in a way that requires at least one of password/accessToken
} | ||
|
||
get rootImportMailFolderName(): Stream<string> { | ||
if (this._rootImportMailFolderName().length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably not correct
@@ -0,0 +1,163 @@ | |||
import stream from "mithril/stream" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some missing validations for this model as well as missing error handling for broken imap server etc
assertMainOrNode() | ||
|
||
export type AddImapImportData = { | ||
model: ImapImportModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this type indirection is unnecessary
|
||
onStartSyncSessionProcess(processId: number, nextMailboxToDownload: ImapSyncSessionMailbox): void { | ||
if (this.state == SyncSessionState.RUNNING) { | ||
console.log("onStartSyncSessionProcess : processId: " + processId + " -> " + nextMailboxToDownload.mailboxState.path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concat in console.log
break | ||
case ImapMailboxSpecialUse.TRASH: | ||
case ImapMailboxSpecialUse.ARCHIVE: | ||
case ImapMailboxSpecialUse.ALL: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we skip this, since it would give us all mails again?
this.adSyncOptimizer.optimizedSyncSessionMailbox.mailCount, | ||
) | ||
.then((deletedUids) => { | ||
this.handleDeletedUids(deletedUids, openedImapMailbox, adSyncEventListener) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apparently, it's intentional to prevent the imap server from limiting us because of repetitive commands
nextUidFetchRequest.uidFetchSequenceString, | ||
{ | ||
uid: true, | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh
}) | ||
} | ||
|
||
private setupImapFlowErrorHandler(imapClient: ImapFlow, adSyncEventListener: AdSyncEventListener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be made use-once
Draft pull request for single user email import.
Notes:
ImapAdSync.ts
as well asImapImporter.ts
.